Extend documentation with a guide for plan resolvers (#244)#2985
Extend documentation with a guide for plan resolvers (#244)#2985msotnikov wants to merge 2 commits intographile:mainfrom
Conversation
|
benjie
left a comment
benjie
left a comment
There was a problem hiding this comment.
Thanks for raising this PR! I've raised a number of comments on the content, let me know your thoughts 🙌 In particular I want to avoid implying that loadOne/loadMany are "custom" steps or that lambda is the only/main step, so I've been a bit tighter around the wording there.
Consider breaking this PR up into smaller pieces; then we can get some of the more obvious parts merged whilst the more subtle pieces continue to be worked on.
| if (isDev && !fn.name && !warnedCallbacks.has(fn)) { | ||
| warnedCallbacks.add(fn); | ||
| console.warn( | ||
| `lambda() was called with an anonymous (inline) callback function. ` + | ||
| `This prevents deduplication. Define the callback at file scope or ` + | ||
| `give it a name for better optimization. ` + | ||
| `See: https://grafast.org/grafast/standard-steps/lambda#define-callback-in-top-scope`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
I'm not super keen on this; I'd rather handle it with a lint rule later. The main reason is that just not having a name is not sufficient to know that it was inline; for example we recommend EXPORTABLE(...) usage everywhere; however:
const { EXPORTABLE } = require('graphile-export')
const TWO = 2
const addTwo = EXPORTABLE((TWO) => (n) => n + TWO, [TWO])
console.log(addTwo.name)This'll output '' since there's no name for this function.
| ## Extract arguments deeply | ||
|
|
||
| When accessing nested argument values, prefer extracting the leaf value directly | ||
| rather than extracting an intermediate object and then pulling values from it. | ||
| This gives Gra*fast* more information about what you actually need, which | ||
| enables better optimization. | ||
|
|
||
| ```graphql | ||
| input UserFilter { | ||
| author: String | ||
| publishedAfter: Int | ||
| } | ||
|
|
||
| type Query { | ||
| bookCount(search: String, filter: UserFilter): Int! | ||
| } | ||
| ``` | ||
|
|
||
| ### Don't: shallow extraction then transform | ||
|
|
||
| ```ts | ||
| function bookCount_plan($parent, fieldArgs) { | ||
| const $filter = fieldArgs.getRaw("filter"); | ||
| // ✘ Creates an unnecessary intermediate lambda step | ||
| const $author = lambda($filter, (f) => f?.author); | ||
| // ... | ||
| } | ||
| ``` | ||
|
|
||
| ### Do: deep extraction directly | ||
|
|
||
| ```ts | ||
| function bookCount_plan($parent, fieldArgs) { | ||
| // ✔ One step, directly optimizable | ||
| const $author = fieldArgs.getRaw(["filter", "author"]); | ||
| const $publishedAfter = fieldArgs.getRaw(["filter", "publishedAfter"]); | ||
| // ... | ||
| } | ||
| ``` | ||
|
|
||
| You can also use the `$`-prefixed shortcut for the same result: | ||
|
|
||
| ```ts | ||
| function bookCount_plan($parent, fieldArgs) { | ||
| const { $search, $filter } = fieldArgs; | ||
| const { $author, $publishedAfter } = $filter; | ||
| // ... | ||
| } | ||
| ``` | ||
|
|
||
| Both `.getRaw()` with a path array and the `$`-prefixed destructuring give | ||
| Gra*fast* direct visibility into exactly which leaf values you need, allowing it | ||
| to skip unnecessary work and optimize the plan more aggressively. |
There was a problem hiding this comment.
This was a major concern before the "eliminate eval" logic came in. It's no longer as important, but probably worth keeping as a best practice anyway - it may become more significant again in future optimizations.
| ## When to use something else | ||
|
|
||
| Before reaching for `lambda`, consider whether a better tool exists: | ||
|
|
||
| - **I/O or async work** → use [`loadOne()`](./loadOne.md) or | ||
| [`loadMany()`](./loadMany.md) which support batching | ||
| - **Non-trivial transforms that appear in multiple fields** → create a | ||
| [custom step class](../step-classes.mdx) with `deduplicate()` support | ||
| - **Side effects** → use [`sideEffect()`](/grafast/standard-steps/sideEffect) | ||
|
|
||
| `lambda` is best reserved for trivial, synchronous, pure transforms such as | ||
| string concatenation or simple arithmetic. | ||
|
|
||
| See [Plan resolver best practices](../plan-resolvers/best-practices.md) for | ||
| more guidance. | ||
|
|
There was a problem hiding this comment.
We already have two notes on this page warning about lambda not batching; perhaps this should be folded into those?
Co-authored-by: Benjie <benjie@jemjie.com>
2 participants
Description
Addresses graphile/crystal-pre-merge#244 — general recommendations when writing plan resolvers.
Documentation:
grafast/website/grafast/plan-resolvers/best-practices.md) covering the four recommendations from the issue:getRaw(["path", "to", "value"])or$-prefixed destructuring instead of shallow extraction followed by lambda transformslambda— comparison table, guidance on when lambda is appropriate vs when to create a step class, worked example of a custom steptry/catchin plan resolvers — why imperative error handling doesn't fit the declarative model, with examples usinginhibitOnNull()andtrap()insteadlambda.mdpointing users towardloadOne/loadMany, custom steps, andsideEffect()before they reach for lambdaplan-resolvers/index.mdxRuntime warning:
NODE_ENV=development),lambda()now emits aconsole.warnwhen called with an anonymous (unnamed) callback function, since such callbacks cannot be deduplicated. The warning fires once perunique callback reference (tracked via
WeakSet).Performance impact
No impact in production. In development mode, a single
WeakSetlookup is added perlambda()call (negligible).Security impact
None.
Checklist
yarn lint:fixpasses.yarn testpasses.RELEASE_NOTES.mdfile (if one exists).